Upstream sync: provider registry, configurable base dir, UI fixes#30
Merged
aaditagrawal merged 9 commits intomainfrom Mar 21, 2026
Merged
Upstream sync: provider registry, configurable base dir, UI fixes#30aaditagrawal merged 9 commits intomainfrom
aaditagrawal merged 9 commits intomainfrom
Conversation
Co-authored-by: Mohammed Muzammil Anwar <muzammil@pluginhive.com> Co-authored-by: Julius Marminge <julius0216@outlook.com>
…ngdotgg#1121) Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
github-actions
bot
added
size:XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors filesystem config from a single Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Resolver as resolveBaseDir
participant Deriver as deriveServerPaths
participant ServerConfig
participant FS as FileSystem Service
participant SQLite as Persistence
CLI->>Resolver: parse --home-dir / env.T3CODE_HOME
Resolver->>Deriver: return baseDir
Deriver->>ServerConfig: compute stateDir, dbPath, attachmentsDir, logsDir
ServerConfig->>FS: ensure dirs (stateDir, attachmentsDir, logsDir)
ServerConfig->>SQLite: provide dbPath to init persistence
CLI->>ServerConfig: start server with derived paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/components/chat/ProviderModelPicker.tsx (1)
271-282:⚠️ Potential issue | 🟡 MinorRemove unused
handleModelChangefunction; inconsistent resolver usage.The
handleModelChangefunction (lines 271–282) is defined but never invoked in this component. All actual model selection logic usesonModelSelect(line 337), which calls the localresolveModelForProviderPickerinstead of the importedresolveSelectableModel. This creates an unused function and leaves a partially completed migration.Either remove
handleModelChangeor migrateonModelSelectto useresolveSelectableModel(ensuring cursor-specific family handling is preserved).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ProviderModelPicker.tsx` around lines 271 - 282, There is an unused function handleModelChange and inconsistent resolver usage: either delete handleModelChange or consolidate selection logic by updating onModelSelect to call the imported resolveSelectableModel(provider, value, props.modelOptionsByProvider[provider]) instead of resolveModelForProviderPicker, then remove handleModelChange; if you migrate, ensure you preserve existing cursor-specific family handling currently done in resolveModelForProviderPicker by applying the same post-resolution mapping (e.g., translate cursor family/model identifiers into the expected selectable model shape) before calling props.onProviderModelChange(provider, resolvedModel) and closing the menu.apps/web/src/composerDraftStore.ts (1)
104-110:⚠️ Potential issue | 🟠 MajorDon't tighten the persisted draft schema without a compatible decode path.
These fields are now required on disk, but the store version is still
2.readPersistedAttachmentIdsFromStorage()later decodes the whole payload viaPersistedComposerDraftStoreStorage, so a pre-upgrade snapshot that lacksstickyModel/stickyModelOptionsis treated as empty until some later write rewrites the key. That can make persisted attachments disappear on the first launch after upgrading. Keep the decode path tolerant of missing sticky fields, or bump the persisted version and migrate before that helper reads it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/composerDraftStore.ts` around lines 104 - 110, PersistedComposerDraftStoreState was tightened to require stickyModel and stickyModelOptions which breaks decoding older on-disk data read by readPersistedAttachmentIdsFromStorage via PersistedComposerDraftStoreStorage; either relax the schema back to accept missing fields (e.g., make stickyModelOptions optional/NullOr) or add a migration bumping the persisted store version and migrate existing payloads before readPersistedAttachmentIdsFromStorage runs so pre-upgrade snapshots aren’t treated as empty. Update the schema or add a versioned decode/migration path in the code that constructs/uses PersistedComposerDraftStoreStorage and ensure readPersistedAttachmentIdsFromStorage uses that tolerant path.apps/server/src/main.ts (1)
91-104:⚠️ Potential issue | 🟠 MajorHandle legacy
T3CODE_STATE_DIR/--state-direxplicitly.The env half now only reads
T3CODE_HOME, and the CLI half only accepts--home-dir. Existing launch scripts that still setT3CODE_STATE_DIRwill silently fall back to the default home directory, while--state-dirnow hard-fails parsing. That can look like data loss because the server starts against a different state root. Please keep a deprecation shim or fail fast with a clear migration message when the legacy inputs are present.Also applies to: 298-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/main.ts` around lines 91 - 104, The code currently only reads T3CODE_HOME (Config.string("T3CODE_HOME") in CliEnvConfig) and the CLI only accepts --home-dir, so legacy T3CODE_STATE_DIR / --state-dir are ignored or hard-fail; update the config and CLI parsing to detect the legacy inputs and provide a clear migration path: in CliEnvConfig add detection for Config.string("T3CODE_STATE_DIR") and if present map it to the t3Home value while emitting a deprecation warning (or, if both T3CODE_HOME and T3CODE_STATE_DIR are set and conflict, fail fast with a clear error message telling the user to migrate to T3CODE_HOME), and in the CLI arg parsing (the block referred at ~298-300) accept --state-dir as an alias for --home-dir or produce a descriptive error instructing users to switch to --home-dir; ensure all messages include the exact legacy key/flag names and recommended replacements so behavior is explicit and no silent fallback occurs.
🧹 Nitpick comments (4)
.gitignore (1)
20-21: LGTM! Consider cleaning up the redundant pattern.The new ignore patterns are correct:
.vitest-*properly ignores Vitest cache/temporary files__screenshots__/correctly ignores screenshot directories (trailing slash ensures directory-only matching)However, line 19 (
apps/web/src/components/__screenshots__) is now redundant since line 21 matches all__screenshots__/directories throughout the repository.♻️ Optional cleanup to remove redundancy
apps/web/.playwright apps/web/playwright-report -apps/web/src/components/__screenshots__ .vitest-* __screenshots__/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 20 - 21, Remove the redundant explicit ignore entry "apps/web/src/components/__screenshots__" since the broader "__screenshots__/" pattern already ignores all such directories; update .gitignore by deleting the specific apps/web/src/components/__screenshots__ line and keep the general "__screenshots__/" and ".vitest-*" entries to avoid duplication.apps/web/src/components/Sidebar.tsx (1)
1853-1861: Consider memoizingprojectStatusto avoid redundant re-computation.The current implementation calls
derivePendingApprovalsandderivePendingUserInputsfor every thread in every project on each render. For projects with many threads or threads with large activity arrays, this could become a performance bottleneck.Consider extracting this computation into a
useMemohook or a separate memoized selector to avoid redundant work when unrelated state changes trigger re-renders.Example memoization approach
You could create a memoized map of project statuses outside the render loop:
const projectStatusById = useMemo(() => { const map = new Map<ProjectId, ReturnType<typeof resolveProjectStatusIndicator>>(); for (const project of projects) { const projectThreads = threads.filter((t) => t.projectId === project.id); const status = resolveProjectStatusIndicator( projectThreads.map((thread) => resolveThreadStatusPill({ thread, hasPendingApprovals: derivePendingApprovals(thread.activities).length > 0, hasPendingUserInput: derivePendingUserInputs(thread.activities).length > 0, }), ), ); map.set(project.id, status); } return map; }, [projects, threads]);Then use
projectStatusById.get(project.id)inside the render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 1853 - 1861, The projectStatus computation repeatedly calls derivePendingApprovals and derivePendingUserInputs for every thread on each render; wrap the logic that builds projectStatus (the resolveProjectStatusIndicator call that maps over projectThreads and calls resolveThreadStatusPill) in a useMemo (or move to a memoized selector) keyed on the relevant inputs (projects and threads or threads' activities) so you compute a Map<ProjectId, status> once and then lookup with project.id in render; ensure the memo dependencies include projects and threads (or a derived stable key for thread.activities) and keep references to resolveProjectStatusIndicator, resolveThreadStatusPill, derivePendingApprovals, and derivePendingUserInputs unchanged.apps/web/src/hooks/useHandleNewThread.ts (1)
53-59: Redundant aliasing of the same store methods.
setModelandsetDraftModelare aliases for the same function, as aresetProviderandsetDraftProvider. This creates confusion about which should be used where.Consider using a single name for each method throughout the function.
♻️ Suggested simplification
const { clearProjectDraftThreadId, draftsByThreadId, getDraftThread, getDraftThreadByProjectId, setModel, setModelOptions, setProvider, setDraftThreadContext, - setModel: setDraftModel, setProjectDraftThreadId, - setProvider: setDraftProvider, } = useComposerDraftStore.getState();Then use
setModelandsetProviderconsistently below (lines 136-140).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useHandleNewThread.ts` around lines 53 - 59, The destructure in useHandleNewThread pulls duplicate aliases (setModel and setModel: setDraftModel, setProvider and setProvider: setDraftProvider); remove the redundant aliases so each store method appears once (keep setModel and setProvider) and update all usages in this module (e.g., the places currently calling setDraftModel and setDraftProvider) to call setModel and setProvider instead so there’s a single canonical name for each action (check usages around where the draft is initialized and the calls currently at lines ~136-140).apps/web/src/components/ChatView.browser.tsx (1)
1285-1293: Seed sticky state through the public store actions in these tests.Using raw
setState()here bypassessetStickyModel()andsetStickyModelOptions(), so these cases won't catch regressions in the new normalization path that production uses. Prefer setting up the sticky state through the store actions instead.Also applies to: 1331-1339, 1407-1415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.browser.tsx` around lines 1285 - 1293, Replace direct useComposerDraftStore.setState({...}) with the public store actions so the normalization path is exercised: call useComposerDraftStore.getState().setStickyModel("gpt-5.3-codex") and useComposerDraftStore.getState().setStickyModelOptions({ codex: { reasoningEffort: "medium", fastMode: true } }) (or the equivalent action signatures) in the affected test setup blocks (also update the similar occurrences around the other ranges noted) so tests seed stickyModel and stickyModelOptions via setStickyModel and setStickyModelOptions instead of bypassing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docs/scripts.md:
- Around line 6-8: Update the docs to use the new environment variable name:
replace the obsolete T3CODE_STATE_DIR with T3CODE_HOME in the sentence that
describes defaulting dev commands; specifically change the phrase "Dev commands
default `T3CODE_STATE_DIR` to `~/.t3/dev`" to use `T3CODE_HOME` so it matches
the renamed variable used throughout the codebase and remains consistent with
the `--base-dir` example.
In `@apps/desktop/src/main.ts`:
- Around line 64-65: BASE_DIR is currently taken directly from
process.env.T3CODE_HOME which can be relative and cause path drift; normalize it
to an absolute path at startup (e.g., compute BASE_DIR from
process.env.T3CODE_HOME if present, otherwise default to Path.join(OS.homedir(),
".t3"), then pass that value through Path.resolve or Path.normalize to produce
an absolute BASE_DIR) and then derive STATE_DIR from that resolved BASE_DIR so
both desktop and backend use the same absolute path; update the BASE_DIR and
STATE_DIR assignments (symbols: BASE_DIR, STATE_DIR, T3CODE_HOME) accordingly.
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts`:
- Around line 237-246: rootDir is created with makeTempDirectoryScoped but its
scope is not closed in the harness, so the temp directory outlives the harness;
allocate a harness-owned Scope (e.g., harnessScope) and call
makeTempDirectoryScoped within that scope to produce rootDir (or capture the
returned disposal handle) and store the scope/handle on the harness; then update
dispose to close that same harnessScope (or call the temp-directory disposal) so
the temporary directory is reliably cleaned up when dispose runs; refer to
makeTempDirectoryScoped, rootDir, deriveServerPaths, and dispose to locate where
to attach and close the scope.
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 518-526: The temporary baseDir created by ServerConfig.layerTest
(via makeTempDirectoryScoped) is being finalized before manager methods run
because ServerConfigLayer is only provided when constructing manager via
Effect.provide(managerLayer); fix by making the ServerConfigLayer live for the
full manager lifetime—either merge ServerConfigLayer into the managerLayer
composition (e.g., include ServerConfigLayer via Layer.provideMerge when
building gitCoreLayer/managerLayer so it remains part of the returned manager's
scope) or build the manager inside a scoped effect (use
Effect.scoped/Layer.build and run manager methods inside that scope) so the
temporary baseDir created by ServerConfig.layerTest remains alive while manager
methods (like manager.* in tests) execute.
In `@apps/web/src/appSettings.test.ts`:
- Around line 269-315: The tests hard-code expectations for only two providers
but the shared helpers now expose all 8 providers; update the assertions to
reflect the full provider list and their defaults. Replace the expected array in
the MODEL_PROVIDER_SETTINGS test to include all 8 provider.provider values,
update getCustomModelsForProvider expectations to assert the custom model lists
for each of the 8 providers (using their actual names as returned by
MODEL_PROVIDER_SETTINGS), update the defaults object and
getDefaultCustomModelsForProvider assertions to include the 8 corresponding
default keys (e.g., customCodexModels, customClaudeModels, customCopilotModels,
customCursorModels, customOpencodeModels, customGeminiCliModels,
customAmpModels, customKiloModels), adjust the patchCustomModels assertions for
any additional providers if needed, and change the getCustomModelsByProvider
expectation to include entries for all 8 providers mapping to their expected
model arrays.
In `@scripts/dev-runner.ts`:
- Around line 104-114: The resolveBaseDir function currently passes a configured
baseDir directly to Path.resolve, which treats a leading "~" as a literal
segment; update resolveBaseDir to expand a leading tilde before resolving:
either import and call the existing expandHomePath from
apps/server/src/os-jank.ts (preferred) or implement expansion by replacing a
leading "~" with OS.homedir() joined via Path.join, then call Path.resolve on
the expanded value; ensure both places that call resolveBaseDir and the
DEFAULT_T3_HOME fallback behavior remain correct.
---
Outside diff comments:
In `@apps/server/src/main.ts`:
- Around line 91-104: The code currently only reads T3CODE_HOME
(Config.string("T3CODE_HOME") in CliEnvConfig) and the CLI only accepts
--home-dir, so legacy T3CODE_STATE_DIR / --state-dir are ignored or hard-fail;
update the config and CLI parsing to detect the legacy inputs and provide a
clear migration path: in CliEnvConfig add detection for
Config.string("T3CODE_STATE_DIR") and if present map it to the t3Home value
while emitting a deprecation warning (or, if both T3CODE_HOME and
T3CODE_STATE_DIR are set and conflict, fail fast with a clear error message
telling the user to migrate to T3CODE_HOME), and in the CLI arg parsing (the
block referred at ~298-300) accept --state-dir as an alias for --home-dir or
produce a descriptive error instructing users to switch to --home-dir; ensure
all messages include the exact legacy key/flag names and recommended
replacements so behavior is explicit and no silent fallback occurs.
In `@apps/web/src/components/chat/ProviderModelPicker.tsx`:
- Around line 271-282: There is an unused function handleModelChange and
inconsistent resolver usage: either delete handleModelChange or consolidate
selection logic by updating onModelSelect to call the imported
resolveSelectableModel(provider, value, props.modelOptionsByProvider[provider])
instead of resolveModelForProviderPicker, then remove handleModelChange; if you
migrate, ensure you preserve existing cursor-specific family handling currently
done in resolveModelForProviderPicker by applying the same post-resolution
mapping (e.g., translate cursor family/model identifiers into the expected
selectable model shape) before calling props.onProviderModelChange(provider,
resolvedModel) and closing the menu.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 104-110: PersistedComposerDraftStoreState was tightened to require
stickyModel and stickyModelOptions which breaks decoding older on-disk data read
by readPersistedAttachmentIdsFromStorage via PersistedComposerDraftStoreStorage;
either relax the schema back to accept missing fields (e.g., make
stickyModelOptions optional/NullOr) or add a migration bumping the persisted
store version and migrate existing payloads before
readPersistedAttachmentIdsFromStorage runs so pre-upgrade snapshots aren’t
treated as empty. Update the schema or add a versioned decode/migration path in
the code that constructs/uses PersistedComposerDraftStoreStorage and ensure
readPersistedAttachmentIdsFromStorage uses that tolerant path.
---
Nitpick comments:
In @.gitignore:
- Around line 20-21: Remove the redundant explicit ignore entry
"apps/web/src/components/__screenshots__" since the broader "__screenshots__/"
pattern already ignores all such directories; update .gitignore by deleting the
specific apps/web/src/components/__screenshots__ line and keep the general
"__screenshots__/" and ".vitest-*" entries to avoid duplication.
In `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 1285-1293: Replace direct useComposerDraftStore.setState({...})
with the public store actions so the normalization path is exercised: call
useComposerDraftStore.getState().setStickyModel("gpt-5.3-codex") and
useComposerDraftStore.getState().setStickyModelOptions({ codex: {
reasoningEffort: "medium", fastMode: true } }) (or the equivalent action
signatures) in the affected test setup blocks (also update the similar
occurrences around the other ranges noted) so tests seed stickyModel and
stickyModelOptions via setStickyModel and setStickyModelOptions instead of
bypassing them.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 1853-1861: The projectStatus computation repeatedly calls
derivePendingApprovals and derivePendingUserInputs for every thread on each
render; wrap the logic that builds projectStatus (the
resolveProjectStatusIndicator call that maps over projectThreads and calls
resolveThreadStatusPill) in a useMemo (or move to a memoized selector) keyed on
the relevant inputs (projects and threads or threads' activities) so you compute
a Map<ProjectId, status> once and then lookup with project.id in render; ensure
the memo dependencies include projects and threads (or a derived stable key for
thread.activities) and keep references to resolveProjectStatusIndicator,
resolveThreadStatusPill, derivePendingApprovals, and derivePendingUserInputs
unchanged.
In `@apps/web/src/hooks/useHandleNewThread.ts`:
- Around line 53-59: The destructure in useHandleNewThread pulls duplicate
aliases (setModel and setModel: setDraftModel, setProvider and setProvider:
setDraftProvider); remove the redundant aliases so each store method appears
once (keep setModel and setProvider) and update all usages in this module (e.g.,
the places currently calling setDraftModel and setDraftProvider) to call
setModel and setProvider instead so there’s a single canonical name for each
action (check usages around where the draft is initialized and the calls
currently at lines ~136-140).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65d9b5f5-4b15-47e2-882d-08d9b5328a37
📒 Files selected for processing (61)
.docs/scripts.md.gitignoreREMOTE.mdapps/desktop/src/main.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/src/attachmentPaths.tsapps/server/src/attachmentStore.test.tsapps/server/src/attachmentStore.tsapps/server/src/config.tsapps/server/src/git/Layers/CodexTextGeneration.test.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/keybindings.test.tsapps/server/src/main.test.tsapps/server/src/main.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/os-jank.tsapps/server/src/persistence/Layers/Sqlite.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/serverLayers.tsapps/server/src/serverLogger.tsapps/server/src/telemetry/Identify.tsapps/server/src/telemetry/Layers/AnalyticsService.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ClaudeTraitsPicker.browser.tsxapps/web/src/components/chat/ClaudeTraitsPicker.tsxapps/web/src/components/chat/CodexTraitsPicker.browser.tsxapps/web/src/components/chat/CodexTraitsPicker.tsxapps/web/src/components/chat/ProviderHealthBanner.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/routes/_chat.settings.tsxpackages/shared/src/model.test.tspackages/shared/src/model.tsscripts/dev-runner.test.tsscripts/dev-runner.tsturbo.json
Comment on lines
6
to
+8
| - Dev commands default `T3CODE_STATE_DIR` to `~/.t3/dev` to keep dev state isolated from desktop/prod state. | ||
| - Override server CLI-equivalent flags from root dev commands with `--`, for example: | ||
| `bun run dev -- --state-dir ~/.t3/another-dev-state` | ||
| `bun run dev -- --base-dir ~/.t3-2` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if T3CODE_STATE_DIR is still used anywhere in the codebase
echo "=== Searching for T3CODE_STATE_DIR usage ==="
rg -n "T3CODE_STATE_DIR" --type-add 'config:*.{json,md,ts,js}' -t config
echo ""
echo "=== Searching for T3CODE_HOME usage ==="
rg -n "T3CODE_HOME" --type-add 'config:*.{json,md,ts,js}' -t configRepository: aaditagrawal/t3code
Length of output: 1631
Update T3CODE_STATE_DIR to T3CODE_HOME on line 6.
The environment variable was renamed to T3CODE_HOME throughout the codebase. Line 6 still references the obsolete T3CODE_STATE_DIR, which no longer exists in the project. Update to match the current variable name and be consistent with line 8's correct usage of --base-dir.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.docs/scripts.md around lines 6 - 8, Update the docs to use the new
environment variable name: replace the obsolete T3CODE_STATE_DIR with
T3CODE_HOME in the sentence that describes defaulting dev commands; specifically
change the phrase "Dev commands default `T3CODE_STATE_DIR` to `~/.t3/dev`" to
use `T3CODE_HOME` so it matches the renamed variable used throughout the
codebase and remains consistent with the `--base-dir` example.
Comment on lines
+64
to
+65
| const BASE_DIR = process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3"); | ||
| const STATE_DIR = Path.join(BASE_DIR, "userdata"); |
There was a problem hiding this comment.
Resolve BASE_DIR to an absolute path to avoid desktop/backend path drift.
If T3CODE_HOME is relative, desktop log paths can diverge from backend paths (different CWDs). Normalize once at startup.
Suggested fix
-const BASE_DIR = process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3");
+const BASE_DIR = Path.resolve(process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3"));
const STATE_DIR = Path.join(BASE_DIR, "userdata");Also applies to: 932-933
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main.ts` around lines 64 - 65, BASE_DIR is currently taken
directly from process.env.T3CODE_HOME which can be relative and cause path
drift; normalize it to an absolute path at startup (e.g., compute BASE_DIR from
process.env.T3CODE_HOME if present, otherwise default to Path.join(OS.homedir(),
".t3"), then pass that value through Path.resolve or Path.normalize to produce
an absolute BASE_DIR) and then derive STATE_DIR from that resolved BASE_DIR so
both desktop and backend use the same absolute path; update the BASE_DIR and
STATE_DIR assignments (symbols: BASE_DIR, STATE_DIR, T3CODE_HOME) accordingly.
Comment on lines
+237
to
+246
| const rootDir = yield* fileSystem.makeTempDirectoryScoped({ | ||
| prefix: "t3-orchestration-integration-", | ||
| }); | ||
| const workspaceDir = path.join(rootDir, "workspace"); | ||
| const stateDir = path.join(rootDir, "state"); | ||
| const dbPath = path.join(stateDir, "state.sqlite"); | ||
| fs.mkdirSync(workspaceDir, { recursive: true }); | ||
| fs.mkdirSync(stateDir, { recursive: true }); | ||
| initializeGitWorkspace(workspaceDir); | ||
| const { stateDir, dbPath } = yield* deriveServerPaths(rootDir, undefined).pipe( | ||
| Effect.provideService(Path.Path, path), | ||
| ); | ||
| yield* fileSystem.makeDirectory(workspaceDir, { recursive: true }); | ||
| yield* fileSystem.makeDirectory(stateDir, { recursive: true }); | ||
| yield* initializeGitWorkspace(workspaceDir); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'makeTempDirectoryScoped|Scope\.make|Scope\.close|dispose = Effect\.gen' apps/server/integration/OrchestrationEngineHarness.integration.tsRepository: aaditagrawal/t3code
Length of output: 1233
Tie the temp directory to the harness-owned lifecycle.
rootDir is acquired with makeTempDirectoryScoped, but dispose only closes the reactor scope (line 353 and 464). The temp directory lifetime is now decoupled from the harness lifecycle: it will be cleaned up by the outer generator scope, not by dispose. Create rootDir under a harness-owned Scope and close that same scope in dispose, or restore explicit cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts` around
lines 237 - 246, rootDir is created with makeTempDirectoryScoped but its scope
is not closed in the harness, so the temp directory outlives the harness;
allocate a harness-owned Scope (e.g., harnessScope) and call
makeTempDirectoryScoped within that scope to produce rootDir (or capture the
returned disposal handle) and store the scope/handle on the harness; then update
dispose to close that same harnessScope (or call the temp-directory disposal) so
the temporary directory is reliably cleaned up when dispose runs; refer to
makeTempDirectoryScoped, rootDir, deriveServerPaths, and dispose to locate where
to attach and close the scope.
Comment on lines
+518
to
526
| const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), { | ||
| prefix: "t3-git-manager-test-", | ||
| }); | ||
|
|
||
| const gitCoreLayer = GitCoreLive.pipe( | ||
| Layer.provideMerge(GitServiceLive), | ||
| Layer.provideMerge(NodeServices.layer), | ||
| Layer.provideMerge(ServerConfigLayer), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Effect-TS, if a Layer allocates a scoped resource (for example via makeTempDirectoryScoped) and that layer is supplied with Effect.provide(effect, layer), is the scoped resource finalized when the provided effect completes?
💡 Result:
Yes—when you do Effect.provide(effect, layer), the layer is built into a scope tied to the lifetime of the provided effect, so any scoped resources the layer acquires (e.g. via makeTempDirectoryScoped / Layer.scoped(...)) are finalized when that effect finishes (success, failure, or interruption), because the scope is closed at that point and layer resources are released on scope close. [1] [2]
The main exception is if you intentionally extend the layer’s scope (e.g. Layer.extendScope(layer)), in which case the resources are not released when the provided effect completes, but only when the outer scope is closed. [3]
Sources:
[1] https://effect-ts.github.io/effect/effect/Layer.ts.html
[2] https://effect-ts.github.io/effect/effect/Effect.ts.html
[3] https://effect-ts.github.io/effect/effect/Layer.ts.html
🏁 Script executed:
# Find the ServerConfig implementation to verify if layerTest uses makeTempDirectoryScoped
fd -e ts -e js config | grep -E "(config|Config)" | head -20Repository: aaditagrawal/t3code
Length of output: 279
🏁 Script executed:
# Search for ServerConfig.layerTest definition
rg -A 10 "layerTest" --type tsRepository: aaditagrawal/t3code
Length of output: 37350
🏁 Script executed:
# Look for makeTempDir and makeTempDirectoryScoped definitions
rg "makeTempDir" --type tsRepository: aaditagrawal/t3code
Length of output: 10867
🏁 Script executed:
# Examine the actual test file to confirm the code context
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '500,550p'Repository: aaditagrawal/t3code
Length of output: 2306
🏁 Script executed:
# Check how makeManager() is called in the tests
rg "makeManager\(\)" apps/server/src/git/Layers/GitManager.test.ts -A 3 | head -40Repository: aaditagrawal/t3code
Length of output: 1171
🏁 Script executed:
# Look at the full context of makeManager usage - check if manager is used after Effect.runPromise or similar
rg "manager\." apps/server/src/git/Layers/GitManager.test.ts -B 2 -A 1 | head -50Repository: aaditagrawal/t3code
Length of output: 1095
🏁 Script executed:
# Verify the actual signature and behavior of makeTempDir in GitManager.test.ts
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '490,510p'Repository: aaditagrawal/t3code
Length of output: 785
🏁 Script executed:
# Check if there are any other test patterns that properly handle scoped resources for comparison
rg "Effect.gen.*makeTempDir" apps/server/src/git/Layers/GitManager.test.ts -B 2 -A 10 | head -60Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Find the makeTempDir function definition in GitManager.test.ts
rg -A 5 "^function makeTempDir" apps/server/src/git/Layers/GitManager.test.tsRepository: aaditagrawal/t3code
Length of output: 349
🏁 Script executed:
# Check actual test examples showing how makeManager() is used and how the manager is consumed
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '543,580p'Repository: aaditagrawal/t3code
Length of output: 1709
🏁 Script executed:
# Check if there are other test files that handle scoped resources correctly by examining similar patterns
rg "Effect.gen.*yield\*.*makeManager" apps/server/src/git/Layers/GitManager.test.ts -B 5 -A 15 | head -80Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Look at how other test helpers in OrchestrationEngine handle baseDir allocation to see the correct pattern
grep -A 30 "function makeManager" apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts | head -40Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Search for how baseDir is allocated in other tests - look at ProviderCommandReactor test
rg -B 5 -A 20 "ServerConfig.layerTest.*baseDir" apps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsRepository: aaditagrawal/t3code
Length of output: 1245
🏁 Script executed:
# Check how the return type of makeManager is used - is it an Effect or a raw value?
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '535,540p'Repository: aaditagrawal/t3code
Length of output: 229
Keep the test baseDir alive for the full manager lifetime.
ServerConfig.layerTest(..., { prefix }) allocates baseDir via makeTempDirectoryScoped, but here that layer is only used inside Effect.provide(managerLayer) to construct and return manager. The scoped resource is finalized when that effect completes, before later manager.* calls run, which makes the worktree-path tests operate against a deleted base directory.
🛠️ Suggested fix
function makeManager(input?: {
ghScenario?: FakeGhScenario;
textGeneration?: Partial<FakeGitTextGeneration>;
sessionTextGeneration?: Partial<FakeGitTextGeneration>;
}) {
- const { service: gitHubCli, ghCalls } = createGitHubCliWithFakeGh(input?.ghScenario);
- const textGeneration = createTextGeneration(input?.textGeneration);
- const sessionTextGeneration = createSessionTextGeneration(input?.sessionTextGeneration);
- const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), {
- prefix: "t3-git-manager-test-",
- });
-
- const gitCoreLayer = GitCoreLive.pipe(
- Layer.provideMerge(GitServiceLive),
- Layer.provideMerge(NodeServices.layer),
- Layer.provideMerge(ServerConfigLayer),
- );
-
- const managerLayer = Layer.mergeAll(
- Layer.succeed(GitHubCli, gitHubCli),
- Layer.succeed(TextGeneration, textGeneration),
- Layer.succeed(SessionTextGeneration, sessionTextGeneration),
- gitCoreLayer,
- ).pipe(Layer.provideMerge(NodeServices.layer));
-
- return makeGitManager.pipe(
- Effect.provide(managerLayer),
- Effect.map((manager) => ({ manager, ghCalls })),
- );
+ return Effect.gen(function* () {
+ const baseDir = yield* makeTempDir("t3-git-manager-test-");
+ const { service: gitHubCli, ghCalls } = createGitHubCliWithFakeGh(input?.ghScenario);
+ const textGeneration = createTextGeneration(input?.textGeneration);
+ const sessionTextGeneration = createSessionTextGeneration(input?.sessionTextGeneration);
+ const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), baseDir);
+
+ const gitCoreLayer = GitCoreLive.pipe(
+ Layer.provideMerge(GitServiceLive),
+ Layer.provideMerge(NodeServices.layer),
+ Layer.provideMerge(ServerConfigLayer),
+ );
+
+ const managerLayer = Layer.mergeAll(
+ Layer.succeed(GitHubCli, gitHubCli),
+ Layer.succeed(TextGeneration, textGeneration),
+ Layer.succeed(SessionTextGeneration, sessionTextGeneration),
+ gitCoreLayer,
+ ).pipe(Layer.provideMerge(NodeServices.layer));
+
+ const manager = yield* makeGitManager.pipe(Effect.provide(managerLayer));
+ return { manager, ghCalls };
+ });
}Also applies to: 528-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 518 - 526, The
temporary baseDir created by ServerConfig.layerTest (via
makeTempDirectoryScoped) is being finalized before manager methods run because
ServerConfigLayer is only provided when constructing manager via
Effect.provide(managerLayer); fix by making the ServerConfigLayer live for the
full manager lifetime—either merge ServerConfigLayer into the managerLayer
composition (e.g., include ServerConfigLayer via Layer.provideMerge when
building gitCoreLayer/managerLayer so it remains part of the returned manager's
scope) or build the manager inside a scoped effect (use
Effect.scoped/Layer.build and run manager methods inside that scope) so the
temporary baseDir created by ServerConfig.layerTest remains alive while manager
methods (like manager.* in tests) execute.
…e expansion in dev-runner
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/dev-runner.ts`:
- Around line 110-113: The expansion logic for configured only handles "~" and
"~/..." and misses Windows-style "~\..." inputs; update the check used when
computing expanded (the conditional that sets expanded from configured) to
detect a tilde followed by either a forward or backslash (e.g., use a test like
configured === "~" || /^~[\\/]/.test(configured)) and then perform the same
expansion by joining homedir() with configured.slice(1) so both "~/path" and
"~\path" are normalized and resolved on Windows and POSIX in the code that
computes expanded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b10b806-8679-4320-8f6b-f6b16c8984e2
📒 Files selected for processing (2)
apps/web/src/appSettings.test.tsscripts/dev-runner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/appSettings.test.ts
Comment on lines
+110
to
+113
| const expanded = | ||
| configured.startsWith("~/") || configured === "~" | ||
| ? path.join(homedir(), configured.slice(1)) | ||
| : configured; |
There was a problem hiding this comment.
Handle Windows-style ~\... home-dir inputs too.
Line 111 only expands ~ and ~/.... ~\... currently bypasses expansion and resolves incorrectly on Windows shells.
💡 Proposed patch
- const expanded =
- configured.startsWith("~/") || configured === "~"
- ? path.join(homedir(), configured.slice(1))
- : configured;
+ const expanded =
+ configured === "~" || configured.startsWith("~/") || configured.startsWith("~\\")
+ ? path.join(homedir(), configured.slice(1))
+ : configured;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-runner.ts` around lines 110 - 113, The expansion logic for
configured only handles "~" and "~/..." and misses Windows-style "~\..." inputs;
update the check used when computing expanded (the conditional that sets
expanded from configured) to detect a tilde followed by either a forward or
backslash (e.g., use a test like configured === "~" ||
/^~[\\/]/.test(configured)) and then perform the same expansion by joining
homedir() with configured.slice(1) so both "~/path" and "~\path" are normalized
and resolved on Windows and POSIX in the code that computes expanded.
… model seeding, skip incompatible picker test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integrates 7 upstream commits from
pingdotgg/t3code, preserving all 8 providers.Upstream commits cherry-picked:
bc124217— Show project status dots when collapsed in sidebar (Show project status dots when collapsed in the sidebar pingdotgg/t3code#1097)46e63b36— fix(ChatView): scroll button style improvement (fix(ChatView): update scroll button styles to improve user interaction pingdotgg/t3code#1246)25c70032— fix(web): health banner for selected provider (fix(web): show health banner for selected provider instead of defaulting to Codex pingdotgg/t3code#1223)34cef620— fix(web): add-project toggle label in cancel state (fix(web): update add-project toggle label in cancel state pingdotgg/t3code#1248)ef256919— Prefer Claude user ID for telemetry identity (Prefer Claude user ID for telemetry identity pingdotgg/t3code#1249)9e29c9d7— feat: configurable base directory (feat: added configurable base directory. pingdotgg/t3code#826) — 37 files, refactors all path derivationd3733897— feat(web): persist modelOptions, refactor provider-specific logic (feat(web): persist modelOptions, refactor provider specific logic pingdotgg/t3code#1121) — 20 files, introducescomposerProviderRegistryKey integration decisions:
Configurable base directory — Clean cherry-pick. Updated our
CopilotAdapterto use newattachmentsDirparameter (upstream only had codex + claude adapters to update).Provider registry — Adopted upstream's
composerProviderRegistrypattern (mapsProviderKind→ state/rendering functions) and extended it with entries for all 8 providers. Providers without custom traits pickers (copilot, opencode, geminiCli, amp, kilo) get minimal entries returning null. Cursor gets a custom entry preserving ourCursorTraitsPicker.appSettings — Took upstream's
ProviderCustomModelConfigandMODEL_PROVIDER_SETTINGSpatterns, extended both to all 8 providers. Kept ourwithDefaultshelper (handles both construction and decoding defaults — technically better than upstream's approach). Kept ourmigratePersistedAppSettingswithproviderAccentColorsandgitTextGenerationModelByProviderlegacy key migrations.composerDraftStore — Kept our version (already ahead of upstream with 8-provider
normalizeProviderModelOptionsand sticky model support).Verification
bun typecheck— 7/7 packages passbun lint— 0 errors, 12 warningsTest plan
bun typecheckpassesbun linthas no errorsSummary by CodeRabbit
New Features
Refactor
Documentation
Chores